cmd/promote/dynatrace/dt_utils.go (1)

108-124: Reset module caches in GetModulesNames before repopulating.

ModulesSlice and ModulesFilesMap are package globals and currently accumulate across repeated calls.

Proposed fix

 func GetModulesNames(baseDir, dir string) ([]string, error) {
+	ModulesSlice = ModulesSlice[:0]
+	ModulesFilesMap = map[string]string{}
+
 	dirGlob := filepath.Join(baseDir, dir)
 	filepaths, err := os.ReadDir(dirGlob)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/promote/dynatrace/dt_utils.go` around lines 108 - 124, GetModulesNames
currently appends to package globals ModulesSlice and ModulesFilesMap on each
call; modify GetModulesNames to reset/clear ModulesSlice and reinitialize or
clear ModulesFilesMap at the start of the function (before reading dir entries)
so repeated calls don't accumulate stale data; locate the function
GetModulesNames and add logic to set ModulesSlice = nil (or empty slice) and
ModulesFilesMap = make(map[string]string) (or clear existing entries) before
populating them.

cmd/promote/utils/service.go (4)

42-44: Inconsistent receiver name on yamlDoc.

GetFilePath (line 38) and Save (line 46) use d, but GetName uses s. Use a consistent receiver name across all methods of the same type.

Proposed fix

-func (s *yamlDoc) GetName() string {
-	return s.name
+func (d *yamlDoc) GetName() string {
+	return d.name
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/promote/utils/service.go` around lines 42 - 44, The GetName method for
type yamlDoc uses an inconsistent receiver name `s`; change its receiver to `d`
to match the other methods on yamlDoc (e.g., GetFilePath and Save) so all
methods use the same receiver identifier; update the method signature for
GetName to use `d` and keep the body returning d.name.

---

413-419: Nit: unconventional blank line in error handling.

The blank line between the call on line 414 and the if err != nil on line 416 breaks the standard Go error-handling idiom. Consider removing it for consistency.

Proposed fix

 	if newHash == "" {
 		newHash, err = repo.GetHeadHash()
-
 		if err != nil {
 			return err
 		}
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/promote/utils/service.go` around lines 413 - 419, Remove the extra blank
line in the error handling after calling repo.GetHeadHash so the idiomatic Go
pattern is preserved; specifically, in the block that assigns newHash (the call
to repo.GetHeadHash) and the subsequent if err != nil block, bring the if
directly under the call (no blank line) to keep the standard newHash, err :=
repo.GetHeadHash / if err != nil style used elsewhere.

---

296-330: Partial failure leaves committed state in the local branch.

Each resourceTemplatePromotion.promote call saves the YAML and commits independently (lines 307, 318). If a later promotion in the loop (line 428) fails, earlier promotions are already committed to the local branch. This is likely acceptable since the user must manually push, but consider documenting this behavior or logging a warning when a partial failure occurs so users know what's been committed.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/promote/utils/service.go` around lines 296 - 330, The promote function
(resourceTemplatePromotion.promote) commits each resource independently (uses
callbacks.SetTargetHash, service.Save, service.appInterfaceClone.Commit) which
can leave the local branch partially committed if a later promotion fails;
update the caller that loops over promotions (the code that invokes
resourceTemplatePromotion.promote) to track which promotions succeeded and, on
any subsequent error, emit a clear warning/log that lists the committed resource
relPaths and informs the user that the local branch contains partial commits
requiring manual push/revert; alternatively expose a boolean or error type from
promote indicating "committed" so the caller can build that list before logging
the warning.

---

21-21: Export the yamlDoc type to match its exported function.

ReadYamlDocFromFile is exported and used externally (e.g., cmd/promote/saas/saas.go:118), but it returns *yamlDoc, which is unexported. This violates Go conventions — external callers cannot declare variables of this type. Export yamlDoc as YamlDoc (or use the lowercase-exported pattern if intentional, but document it).

Additionally, the receiver names for yamlDoc methods are inconsistent: GetFilePath() and Save() use receiver d, while GetName() (line 42) uses receiver s. Standardize on a single receiver name like d.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/promote/utils/service.go` at line 21, The exported function
ReadYamlDocFromFile returns an unexported type yamlDoc which prevents external
callers from using its value; rename the type to exported YamlDoc (or change the
function to return an exported interface) and update all references accordingly
(including ReadYamlDocFromFile's return type and any uses in
cmd/promote/saas/saas.go). Also standardize method receiver names on that type:
change the GetName receiver from 's' to 'd' to match GetFilePath and Save so all
methods on yamlDoc/YamlDoc use a consistent receiver (d). Ensure you update any
imports/usages and tests to the new exported type name.

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

cmd/promote/dynatrace/dynatrace.go (1)

126-131: ⚠️ Potential issue | 🟠 Major

Return the promotion error instead of exiting the process.

os.Exit(1) bypasses Cobra's RunE error path and skips deferred cleanup/tests.

🔧 Proposed fix

-					err = service.Promote(&utils.DefaultPromoteCallbacks{Service: service}, ops.gitHash)
-
-					if err != nil {
-						fmt.Printf("Error while promoting service: %v\n", err)
-						os.Exit(1)
-					}
+					if err := service.Promote(&utils.DefaultPromoteCallbacks{Service: service}, ops.gitHash); err != nil {
+						return fmt.Errorf("error while promoting service: %w", err)
+					}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/promote/dynatrace/dynatrace.go` around lines 126 - 131, The code
currently calls fmt.Printf and os.Exit(1) after service.Promote fails, which
prevents Cobra's RunE error handling and deferred cleanup; change the block in
the function that calls service.Promote(&utils.DefaultPromoteCallbacks{Service:
service}, ops.gitHash) to return the error (or a wrapped error with context)
instead of printing and exiting so the caller/RunE can handle it and deferred
cleanup runs; specifically remove fmt.Printf/os.Exit and return err (or
fmt.Errorf("promote failed: %w", err)) from the surrounding command handler.

cmd/promote/utils/git_repo.go (1)

70-75: Minor: Missing space in warning message.

Line 73 has "Warning:Failed" - should be "Warning: Failed" for consistency with line 40.

✏️ Fix formatting

 func (r *Repo) Cleanup() {
 	err := os.RemoveAll(r.clonePath)
 	if err != nil {
-		fmt.Printf("Warning:Failed to remove clone directory '%s': %v", r.clonePath, err)
+		fmt.Printf("Warning: Failed to remove clone directory '%s': %v", r.clonePath, err)
 	}
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/promote/utils/git_repo.go` around lines 70 - 75, The warning string in
Repo.Cleanup uses "Warning:Failed" without a space; update the fmt.Printf call
inside Cleanup (function Repo.Cleanup) to insert a space after the colon (i.e.,
"Warning: Failed to remove clone directory '%s': %v") so the message matches the
formatting used elsewhere (referencing r.clonePath and the fmt.Printf call).

cmd/promote/dynatrace/dt_utils.go (1)

25-28: Package-level mutable state can cause test pollution.

ModulesSlice and ModulesFilesMap are package-level variables that GetModulesNames appends to. This can cause issues if tests run in parallel or if the function is called multiple times.

Consider returning fresh slices/maps instead of mutating package-level state, or clearing them at the start of GetModulesNames.

Also applies to: 106-123

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/promote/dynatrace/dt_utils.go` around lines 25 - 28, ModulesSlice and
ModulesFilesMap are package-level mutable variables that GetModulesNames appends
to, causing test pollution; change GetModulesNames to use and return fresh local
variables (e.g., local modulesSlice []string and modulesFilesMap
map[string]string) instead of appending to the package-level
ModulesSlice/ModulesFilesMap, and update callers to accept the returned
slice/map, or alternatively clear ModulesSlice and ModulesFilesMap at the start
of GetModulesNames before use; apply the same local/clear fix to the other
similar code block referenced around lines 106-123 (same function or helper) so
no package-level mutation persists across calls/tests.

cmd/promote/utils/app_interface_clone.go (1)

110-128: Consider detecting the default branch dynamically instead of hardcoding master.

The function assumes master is the default branch, which is correct for osdctl. However, many repositories now use main as the default. For robustness, consider detecting the default branch dynamically or making it configurable, so the function doesn't fail if used with repositories that have adopted main.

♻️ Suggested approach

-func (a *AppInterfaceClone) CheckoutNewBranch(branchName string) error {
-	if err := a.workTree.Checkout(&git.CheckoutOptions{Branch: plumbing.NewBranchReferenceName("master")}); err != nil {
-		return fmt.Errorf("failed to checkout master branch in '%s': %v", a.path, err)
+func (a *AppInterfaceClone) CheckoutNewBranch(branchName string, baseBranch string) error {
+	if baseBranch == "" {
+		baseBranch = "master"
+	}
+	if err := a.workTree.Checkout(&git.CheckoutOptions{Branch: plumbing.NewBranchReferenceName(baseBranch)}); err != nil {
+		return fmt.Errorf("failed to checkout %s branch in '%s': %v", baseBranch, a.path, err)
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/promote/utils/app_interface_clone.go` around lines 110 - 128,
CheckoutNewBranch currently hardcodes "master"; change it to detect the
repository's current/default branch instead and use that for the initial
checkout. In CheckoutNewBranch use repo.Head() (or resolve
"refs/remotes/origin/HEAD" if you need remote default) to obtain the default
branch reference name and pass that into workTree.Checkout instead of
plumbing.NewBranchReferenceName("master"), falling back to "master" or "main"
only if detection fails; keep existing logic for branchReference creation,
repo.Reference check, RemoveReference and final Checkout(Create:true).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

cmd/promote/dynatrace/dynatrace.go (2)

93-117: ⚠️ Potential issue | 🟠 Major

Validate flags before opening the app-interface clone.

Line 93 and Line 98 do repo discovery before the --list / --component checks. As a result, --list still goes through the old usage path, and missing-flag invocations can fail on clone/registry setup instead of returning the real CLI error. Move the non-terraform flag validation ahead of FindAppInterfaceClone / NewServicesRegistry.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/promote/dynatrace/dynatrace.go` around lines 93 - 117, The CLI flag
validation for non-terraform operations should run before cloning the repo and
constructing the services registry: move the checks that validate ops.list,
ops.component, and ops.gitHash (the logic that returns errors for "--list cannot
be used with --component or --gitHash" and "--component is required unless
--list is used") above the calls to utils.FindAppInterfaceClone and
utils.NewServicesRegistry; if ops.list is true preserve setting cmd.SilenceUsage
= true and directly call listServiceIds(servicesRegistry) only after
constructing the registry when actually needed, but prevent any repo
clone/registry setup when basic flag validation would already return an error.
Ensure the condition ordering around ops.list, ops.component and ops.gitHash is
performed first so invalid flag combinations short-circuit before invoking
FindAppInterfaceClone or NewServicesRegistry.

---

128-130: ⚠️ Potential issue | 🟠 Major

Return the RunE error instead of exiting the process.

This branch still bypasses Cobra's error handling and makes the command harder to test.

Proposed fix

-					if err != nil {
-						fmt.Printf("Error while promoting service: %v\n", err)
-						os.Exit(1)
-					}
+					if err != nil {
+						return fmt.Errorf("error while promoting service: %w", err)
+					}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/promote/dynatrace/dynatrace.go` around lines 128 - 130, The code inside
the Cobra command's RunE handler (the RunE closure in dynatrace.go that
currently prints the error and calls os.Exit(1)) should not exit the process;
instead remove the fmt.Printf and os.Exit(1) and return the error to let Cobra
handle it. Update the error branch in the RunE closure (the block that checks if
err != nil after promoting the service) to return a wrapped or raw err (e.g.,
fmt.Errorf("promote service: %w", err) or simply return err) so tests and
Cobra's error handling can observe the failure.

cmd/promote/utils/services_registry.go (1)

35-37: ⚠️ Potential issue | 🟠 Major

Reject duplicate service IDs instead of overwriting them.

Because serviceId is just the basename, Line 37 silently replaces an earlier entry if two scanned directories contain the same filename. Since this constructor accepts multiple directories, that can point a promotion at the wrong service file.

Proposed fix

 			if serviceFilePath != "" {
 				serviceId := strings.TrimSuffix(fileName, filepath.Ext(fileName))
+				if existingPath, exists := serviceIdToFilePath[serviceId]; exists && existingPath != serviceFilePath {
+					return nil, fmt.Errorf("duplicate service id %q mapped to both %q and %q", serviceId, existingPath, serviceFilePath)
+				}
 				serviceIdToFilePath[serviceId] = serviceFilePath
 			}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/promote/utils/services_registry.go` around lines 35 - 37, Change the code
that assigns serviceIdToFilePath[serviceId] = serviceFilePath to detect
duplicates and reject them instead of overwriting: before inserting, check if
serviceId already exists in serviceIdToFilePath; if it does, return an error (or
fail construction) that includes the conflicting serviceId and both file paths
(existing and new) so the caller can detect which scanned directories produced
the collision. Update the constructor/loader function that iterates
fileName/serviceFilePath to perform this check and propagate the error back to
callers.

cmd/promote/saas/saas.go (1)

34-45: ⚠️ Potential issue | 🟡 Minor

Don't return a directory as a service file.

Line 44 still returns filePath even when the saas-* entry is a directory and deploy.yaml is missing. That turns an invalid registry entry into a later YAML read/parsing failure instead of skipping it cleanly.

Suggested fix

 func validateSaasServiceFilePath(filePath string) string {
 	if !strings.HasPrefix(filepath.Base(filePath), "saas-") {
 		return ""
 	}
 
 	subFilePath := filepath.Join(filePath, "deploy.yaml")
 	if fileInfo, err := os.Stat(subFilePath); err == nil && fileInfo.Mode().IsRegular() {
 		return subFilePath
 	}
 
-	return filePath
+	if fileInfo, err := os.Stat(filePath); err == nil && fileInfo.Mode().IsRegular() {
+		return filePath
+	}
+
+	return ""
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/promote/saas/saas.go` around lines 34 - 45, validateSaasServiceFilePath
currently can return a directory path when a "saas-*" entry is a directory and
deploy.yaml is missing; change the logic so directories are never returned: in
validateSaasServiceFilePath, after verifying
strings.HasPrefix(filepath.Base(filePath), "saas-"), stat filePath and if it's a
directory then only return filepath.Join(filePath, "deploy.yaml") when that file
exists and is regular (as checked now); if deploy.yaml is missing return "" (do
not return the directory); if filePath itself is a regular file return filePath
as before.

cmd/promote/dynatrace/dt_utils.go (1)

25-28: Make module discovery stateless.

ModulesSlice and ModulesFilesMap are append-only globals, so a second GetModulesNames call in the same process keeps stale entries around. That can duplicate --list output and let ValidateModuleName succeed with data from a previous checkout. Return fresh data from the helper instead of caching it at package scope.

Also applies to: 106-123

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/promote/dynatrace/dt_utils.go` around lines 25 - 28, The globals
ModulesSlice and ModulesFilesMap make module discovery stateful; change
GetModulesNames to allocate and return fresh []string and map[string]string
local variables (do not append to package-scope ModulesSlice/ModulesFilesMap),
update callers (e.g., ValidateModuleName) to accept or call GetModulesNames for
fresh data instead of reading globals, and remove or replace the append-only
globals (and any similar package-scope caches around lines 106-123) so each call
computes a new module list/map.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

cmd/promote/utils/app_interface_clone.go (2)

110-137: ⚠️ Potential issue | 🟠 Major

Don't stage the caller's whole checkout.

AddGlob(".") will sweep in any unrelated tracked or untracked edits already present in the reused app-interface clone. That can produce promotion commits with files this command never touched.

Suggested fix

 func (a *AppInterfaceClone) CheckoutNewBranch(branchName string) error {
+	status, err := a.workTree.Status()
+	if err != nil {
+		return fmt.Errorf("failed to inspect work tree in '%s': %w", a.path, err)
+	}
+	if !status.IsClean() {
+		return fmt.Errorf("refusing to reuse dirty app-interface clone '%s'; commit or stash local changes first", a.path)
+	}
+
 	if err := a.workTree.Checkout(&git.CheckoutOptions{Branch: plumbing.NewBranchReferenceName("master")}); err != nil {
 		return fmt.Errorf("failed to checkout master branch in '%s': %v", a.path, err)
 	}
@@
 	return nil
 }
 
-func (a *AppInterfaceClone) Commit(commitMessage string) error {
-	if err := a.workTree.AddGlob("."); err != nil {
-		return fmt.Errorf("failed to add files to the git index : %v", err)
+func (a *AppInterfaceClone) Commit(commitMessage string, filePaths ...string) error {
+	for _, absPath := range filePaths {
+		relPath, err := filepath.Rel(a.path, absPath)
+		if err != nil {
+			return fmt.Errorf("failed to compute relative path for '%s' in '%s': %w", absPath, a.path, err)
+		}
+		if err := a.workTree.Add(relPath); err != nil {
+			return fmt.Errorf("failed to add '%s' to the git index: %w", relPath, err)
+		}
 	}
 
 	if _, err := a.workTree.Commit(commitMessage, &git.CommitOptions{}); err != nil {
-		return fmt.Errorf("failed to commit changes in '%s': %v", a.path, err)
+		return fmt.Errorf("failed to commit changes in '%s': %w", a.path, err)
 	}
 
 	return nil
 }

Callers will need to pass the files promotion actually wrote.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/promote/utils/app_interface_clone.go` around lines 110 - 137, The Commit
method currently stages everything via a.workTree.AddGlob("."), which will
include unrelated changes; change the signature of Commit to accept an explicit
list of files (e.g., Commit(commitMessage string, files []string) error) and
remove the AddGlob(".") call, then iterate over files and stage each one
individually via a.workTree.Add (or the repo's add method) before calling
a.workTree.Commit; update any callers to pass only the files the promotion
actually wrote. Keep CheckoutNewBranch as-is (it just creates the branch) and
ensure error messages still include a.path and the original error when staging
or committing fails.

---

77-100: ⚠️ Potential issue | 🟠 Major

Keep clone fallback silent on stdout.

The unconditional fmt.Printf runs on the normal cwd→default fallback path. That pollutes CLI output even when clone resolution succeeds, which breaks scripting and list/data output.

Suggested fix

 func FindAppInterfaceClone(providedPath string) (*AppInterfaceClone, error) {
 	if providedPath != "" {
 		appInterfaceClone, err := newAppInterfaceClone(providedPath, nil)
@@
 		return appInterfaceClone, nil
 	}
 
+	var currentDirErr error
 	{
 		currentDirPath, err := os.Getwd()
 		if err == nil {
 			var currentDirRepo *git.Repository
@@
 				}
 			}
 		}
-		fmt.Printf("Current working directory does not appear to be a suitable app-interface clone location: %v\n\n", err)
+		currentDirErr = err
 	}
 
 	{
 		defaultAppInterfacePath := getDefaultAppInterfacePath()
 		appInterfaceClone, err := newAppInterfaceClone(defaultAppInterfacePath, nil)
 		if err != nil {
-			return nil, fmt.Errorf("default directory '%s' is not an app-interface clone location: %v", defaultAppInterfacePath, err)
+			return nil, fmt.Errorf(
+				"failed to resolve app-interface clone from cwd or default path: cwd=%v; default directory '%s': %w",
+				currentDirErr,
+				defaultAppInterfacePath,
+				err,
+			)
 		}
 		return appInterfaceClone, nil
 	}
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/promote/utils/app_interface_clone.go` around lines 77 - 100, The
unconditional fmt.Printf that prints "Current working directory does not
appear..." writes to stdout and should not pollute normal CLI output; change
that call to write to stderr (use fmt.Fprintf(os.Stderr, ...)) or remove it
entirely so the cwd→default fallback is silent. Update the fmt.Printf usage near
the currentDirPath/newAppInterfaceClone block (the error print that uses err) to
fmt.Fprintf(os.Stderr, ...) and keep the rest of the fallback logic
(getDefaultAppInterfacePath and newAppInterfaceClone) unchanged.

cmd/promote/dynatrace/dynatrace.go (1)

90-128: ⚠️ Potential issue | 🟠 Major

Validate the non-terraform flag matrix before opening app-interface.

--list still passes through the SaaS usage path, so it prints the component usage banner before listing, and bad flag combos can fail on clone/registry setup before returning the real CLI error. --module is also silently ignored here, so --component X --module Y still promotes X.

Suggested fix

 		} else {
-			ops.validateSaasFlow()
+			if ops.module != "" {
+				return errors.New("--module can only be used with --terraform")
+			}
+			if ops.list {
+				if ops.component != "" || ops.gitHash != "" {
+					return errors.New("--list cannot be used with --component or --gitHash")
+				}
+			} else if ops.component == "" {
+				return errors.New("--component is required unless --list is used")
+			}
 
 			appInterfaceClone, err := utils.FindAppInterfaceClone(ops.appInterfaceProvidedPath)
 			if err != nil {
 				return err
@@
 			if ops.list {
-				if ops.component != "" || ops.gitHash != "" {
-					return errors.New("--list cannot be used with --component or --gitHash")
-				}
-
 				cmd.SilenceUsage = true
-
 				return listServiceIds(servicesRegistry)
-			} else {
-				if ops.component == "" {
-					return errors.New("--component is required unless --list is used")
-				}
-
-				cmd.SilenceUsage = true
-
-				service, err := servicesRegistry.GetService(ops.component)
-				if err != nil {
-					return err
-				}
-				err = service.Promote(&utils.DefaultPromoteCallbacks{Service: service}, ops.gitHash)
-
-				if err != nil {
-					return fmt.Errorf("error while promoting service: %v", err)
-				}
 			}
+
+			cmd.SilenceUsage = true
+
+			service, err := servicesRegistry.GetService(ops.component)
+			if err != nil {
+				return err
+			}
+			err = service.Promote(&utils.DefaultPromoteCallbacks{Service: service}, ops.gitHash)
+			if err != nil {
+				return fmt.Errorf("error while promoting service: %v", err)
+			}
 		}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/promote/dynatrace/dynatrace.go` around lines 90 - 128, Validate the CLI
flag matrix (ops.list, ops.component, ops.gitHash, ops.module) before calling
utils.FindAppInterfaceClone or utils.NewServicesRegistry: check and return
errors early for invalid combos (e.g., --list cannot be used with --component or
--gitHash; --component is required unless --list is used) and explicitly reject
or handle --module when it would be ignored (e.g., error if ops.module != "" and
ops.component != "" or otherwise document/handle module semantics) so that
listServiceIds, servicesRegistry.GetService, service.Promote and related calls
are only reached after flag validation.

cmd/promote/utils/services_registry.go (1)

35-37: ⚠️ Potential issue | 🟠 Major

Reject colliding serviceIds during registry build.

This registry already merges multiple roots from cmd/promote/saas/saas.go, so the same basename can exist in different trees. On Line 37, the later entry silently wins, which makes --serviceId ambiguous and can resolve the wrong service file.

Possible fix

 			if serviceFilePath != "" {
 				serviceId := strings.TrimSuffix(fileName, filepath.Ext(fileName))
+				if existingPath, exists := serviceIdToFilePath[serviceId]; exists && existingPath != serviceFilePath {
+					return nil, fmt.Errorf("duplicate serviceId %q found in %q and %q", serviceId, existingPath, serviceFilePath)
+				}
 				serviceIdToFilePath[serviceId] = serviceFilePath
 			}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/promote/utils/services_registry.go` around lines 35 - 37, During registry
build, currently the code does serviceId := strings.TrimSuffix(fileName,
filepath.Ext(fileName)) and unconditionally sets serviceIdToFilePath[serviceId]
= serviceFilePath; update that logic to detect collisions: if serviceId already
exists in serviceIdToFilePath and the stored path differs from serviceFilePath,
return an error (or fail) instead of overwriting, including both conflicting
paths and the serviceId in the error message; perform this check where serviceId
and serviceFilePath are computed so --serviceId is unambiguous.

cmd/promote/saas/saas.go (1)

39-44: ⚠️ Potential issue | 🟡 Minor

Return only regular files from validateSaasServiceFilePath.

If filePath is a saas-* directory and deploy.yaml is missing, Line 44 returns the directory anyway. The registry then advertises an entry that ReadServiceFromFile cannot parse.

Possible fix

 	subFilePath := filepath.Join(filePath, "deploy.yaml")
 	if fileInfo, err := os.Stat(subFilePath); err == nil && fileInfo.Mode().IsRegular() {
 		return subFilePath
 	}
 
-	return filePath
+	if fileInfo, err := os.Stat(filePath); err == nil && fileInfo.Mode().IsRegular() {
+		return filePath
+	}
+
+	return ""
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/promote/saas/saas.go` around lines 39 - 44, The function
validateSaasServiceFilePath currently returns filePath even when it's a
directory; change it so you only return a regular file: first check subFilePath
(deploy.yaml) as you do, and if absent then os.Stat(filePath) and only return
filePath if fileInfo.Mode().IsRegular(); otherwise return an empty string (or
nil/error) so the registry won't advertise a directory that ReadServiceFromFile
can't parse. Ensure you update the behavior in validateSaasServiceFilePath and
callers that expect a non-empty file path.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

docs/README.md (1)

4209-4211: ⚠️ Potential issue | 🟡 Minor

Add a language identifier to this fenced block.

This fence is still bare, so markdownlint MD040 will keep failing.

🛠️ Proposed fix

-```
+```text
 osdctl promote managedscripts [flags]

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @docs/README.md around lines 4209 - 4211, The fenced code block containing
"osdctl promote managedscripts [flags]" is missing a language identifier; update
that Markdown fence (the triple-backtick block that wraps that line) to include
an explicit language tag such as text or bash (e.g., change totext) so
markdownlint MD040 stops flagging it; locate the fence that surrounds the string
"osdctl promote managedscripts [flags]" and add the identifier.

</details>

</blockquote></details>
<details>
<summary>cmd/promote/managedscripts/managed_scripts.go (1)</summary><blockquote>

`63-69`: _⚠️ Potential issue_ | _🟠 Major_

**Register `managed-scripts` as the command name.**

The feature is introduced as `managed-scripts`, but `Use: "managedscripts"` only exposes the unhyphenated form. `osdctl promote managed-scripts` will still be unknown.
 

<details>
<summary>🔧 Proposed fix</summary>

```diff
 	cmd := &cobra.Command{
-		Use:               "managedscripts",
+		Use:               "managed-scripts",
+		Aliases:           []string{"managedscripts"},
 		Short:             "Promote https://github.com/openshift/managed-scripts",
@@
-		osdctl promote managedscripts --gitHash <git-hash>`,
+		osdctl promote managed-scripts --gitHash <git-hash>`,
```
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@cmd/promote/managedscripts/managed_scripts.go` around lines 63 - 69, The
cobra command is registered as Use: "managedscripts" but should be hyphenated;
update the command definition (the Cobra command struct where Use is set—e.g.,
the managed scripts command in managed_scripts.go) to Use: "managed-scripts" and
adjust any Example or help text that shows the command invocation to use the
hyphenated form (e.g., "osdctl promote managed-scripts --gitHash <git-hash>") so
the CLI recognizes osdctl promote managed-scripts.
```

</details>

</blockquote></details>
<details>
<summary>cmd/promote/dynatrace/dynatrace.go (1)</summary><blockquote>

`90-104`: _⚠️ Potential issue_ | _🟡 Minor_

**Fail fast before bootstrapping the SaaS flow.**

`validateSaasFlow()` only prints text, so `--list` still emits that banner and missing-flag calls still do repo discovery before the real validation runs.
 

<details>
<summary>♻️ Suggested control-flow cleanup</summary>

```diff
 		} else {
-			ops.validateSaasFlow()
-
-			appInterfaceClone, err := utils.FindAppInterfaceClone(ops.appInterfaceProvidedPath)
-			if err != nil {
-				return err
-			}
-
-			servicesRegistry, err := utils.NewServicesRegistry(
-				appInterfaceClone,
-				validateDynatraceServiceFilePath,
-				saasDynatraceDir,
-			)
-			if err != nil {
-				return err
-			}
-
 			if ops.list {
 				if ops.component != "" || ops.gitHash != "" {
 					return errors.New("--list cannot be used with --component or --gitHash")
 				}
-				cmd.SilenceUsage = true
-				return listServiceIds(servicesRegistry)
-			} else {
-				if ops.component == "" {
-					return errors.New("--component is required unless --list is used")
-				}
+			} else if ops.component == "" {
+				return errors.New("--component is required unless --list is used")
+			}
 
-				cmd.SilenceUsage = true
+			cmd.SilenceUsage = true
+			appInterfaceClone, err := utils.FindAppInterfaceClone(ops.appInterfaceProvidedPath)
+			if err != nil {
+				return err
+			}
+			servicesRegistry, err := utils.NewServicesRegistry(
+				appInterfaceClone,
+				validateDynatraceServiceFilePath,
+				saasDynatraceDir,
+			)
+			if err != nil {
+				return err
+			}
 
+			if ops.list {
+				return listServiceIds(servicesRegistry)
+			} else {
 				service, err := servicesRegistry.GetService(ops.component)
```
</details>

As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."


Also applies to: 106-116

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@cmd/promote/dynatrace/dynatrace.go` around lines 90 - 104, The code currently
calls ops.validateSaasFlow() but continues to bootstrap the repo
(utils.FindAppInterfaceClone / utils.NewServicesRegistry) even when validation
should abort; change validateSaasFlow() to return an error (or a boolean+error)
and update the caller to run it first and return on error before invoking
utils.FindAppInterfaceClone or utils.NewServicesRegistry; propagate the new
error return through ops.validateSaasFlow() calls in this file and the similar
block around the 106-116 region so the command fails fast without doing repo
discovery when validation fails or when flags like --list should short-circuit.
```

</details>

</blockquote></details>
<details>
<summary>cmd/promote/saas/saas.go (1)</summary><blockquote>

`34-45`: _⚠️ Potential issue_ | _🟡 Minor_

**Do not register directories as service files.**

Line 44 returns `filePath` even when it is a directory without `deploy.yaml`. `utils.NewServicesRegistry` treats any non-empty callback result as a valid service path, so `--list` can surface bogus IDs and `GetService` then fails later on a non-file path.
 

<details>
<summary>Suggested fix</summary>

```diff
 func validateSaasServiceFilePath(filePath string) string {
 	if !strings.HasPrefix(filepath.Base(filePath), "saas-") {
 		return ""
 	}
 
 	subFilePath := filepath.Join(filePath, "deploy.yaml")
 	if fileInfo, err := os.Stat(subFilePath); err == nil && fileInfo.Mode().IsRegular() {
 		return subFilePath
 	}
 
-	return filePath
+	if fileInfo, err := os.Stat(filePath); err == nil && fileInfo.Mode().IsRegular() {
+		return filePath
+	}
+
+	return ""
 }
```
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@cmd/promote/saas/saas.go` around lines 34 - 45, validateSaasServiceFilePath
currently returns the input directory path when no deploy.yaml exists, causing
directories to be treated as valid service files; update
validateSaasServiceFilePath to only return a non-empty string for actual files
by checking that either the discovered subFilePath (deploy.yaml) is a regular
file or that the original filePath itself is a regular file (use os.Stat and
fileInfo.Mode().IsRegular()); if neither is a regular file, return an empty
string so utils.NewServicesRegistry and GetService won't register directories as
services.
```

</details>

</blockquote></details>

</blockquote></details>

<details>
<summary>🤖 Prompt for all review comments with AI agents</summary>

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @cmd/promote/dynatrace/dt_utils.go:

• Around line 38-58: getResourceTemplatesPaths currently swallows errors from
  serviceRegistry.GetService and VisitElements and returns an empty string, making
  parse failures indistinguishable from a legitimate no-path case; change its
  signature to return (string, error), propagate and return any error from
  serviceRegistry.GetService and from
  service.GetResourceTemplatesSequenceNode().VisitElements instead of returning
  "", and only return the joined paths and nil error on success; update any
  callers to handle the (string, error) result accordingly (the same pattern
  should be applied to the similar function covering lines 61-82).

In @cmd/promote/saas/saas.go:

• Around line 267-282: The CLI currently only enforces --gitHash for hotfixes
  but still calls service.Promote with an empty ops.gitHash for normal promotes;
  add a guard immediately before calling service.Promote to validate ops.gitHash
  != "" and return an error like "--gitHash is required" if empty so promotion
  fails fast at the CLI boundary; update the block that retrieves service
  (servicesRegistry.GetService / cmd.SilenceUsage) to perform this check and only
  call service.Promote when ops.gitHash is present (references: ops.gitHash,
  ops.isHotfix, service.Promote, promoteCallbacks).

In @cmd/promote/utils/app_interface_clone.go:

• Around line 119-122: CheckoutNewBranch currently hardcodes checking out
  "master" and ignores the branchName parameter, which breaks repos using "main"
  or other default branches; update CheckoutNewBranch to determine and checkout
  the repository's current default branch (or accept it via branchName) instead of
  plumbing.NewBranchReferenceName("master"), e.g. detect the HEAD or remote's
  default ref using the repository or workTree APIs and pass that ref into
  a.workTree.Checkout(&git.CheckoutOptions{Branch: ...}), then create/check out
  the new branch using branchName; ensure you reference CheckoutNewBranch,
  a.workTree.Checkout, plumbing.NewBranchReferenceName and the branchName
  parameter when making the change.

---

Duplicate comments:
In @cmd/promote/dynatrace/dynatrace.go:

• Around line 90-104: The code currently calls ops.validateSaasFlow() but
  continues to bootstrap the repo (utils.FindAppInterfaceClone /
  utils.NewServicesRegistry) even when validation should abort; change
  validateSaasFlow() to return an error (or a boolean+error) and update the caller
  to run it first and return on error before invoking utils.FindAppInterfaceClone
  or utils.NewServicesRegistry; propagate the new error return through
  ops.validateSaasFlow() calls in this file and the similar block around the
  106-116 region so the command fails fast without doing repo discovery when
  validation fails or when flags like --list should short-circuit.

In @cmd/promote/managedscripts/managed_scripts.go:

• Around line 63-69: The cobra command is registered as Use: "managedscripts"
  but should be hyphenated; update the command definition (the Cobra command
  struct where Use is set—e.g., the managed scripts command in managed_scripts.go)
  to Use: "managed-scripts" and adjust any Example or help text that shows the
  command invocation to use the hyphenated form (e.g., "osdctl promote
  managed-scripts --gitHash ") so the CLI recognizes osdctl promote
  managed-scripts.

In @cmd/promote/saas/saas.go:

• Around line 34-45: validateSaasServiceFilePath currently returns the input
  directory path when no deploy.yaml exists, causing directories to be treated as
  valid service files; update validateSaasServiceFilePath to only return a
  non-empty string for actual files by checking that either the discovered
  subFilePath (deploy.yaml) is a regular file or that the original filePath itself
  is a regular file (use os.Stat and fileInfo.Mode().IsRegular()); if neither is a
  regular file, return an empty string so utils.NewServicesRegistry and GetService
  won't register directories as services.

In @docs/README.md:

• Around line 4209-4211: The fenced code block containing "osdctl promote
  managedscripts [flags]" is missing a language identifier; update that Markdown
  fence (the triple-backtick block that wraps that line) to include an explicit
  language tag such as text or bash (e.g., change totext) so markdownlint
  MD040 stops flagging it; locate the fence that surrounds the string "osdctl
  promote managedscripts [flags]" and add the identifier.

</details>

<details>
<summary>🪄 Autofix (Beta)</summary>

Fix all unresolved CodeRabbit comments on this PR:

- [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended)
- [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes

</details>

---

<details>
<summary>ℹ️ Review info</summary>

<details>
<summary>⚙️ Run configuration</summary>

**Configuration used**: Organization UI

**Review profile**: CHILL

**Plan**: Pro

**Run ID**: `df623ab3-2e2d-46f2-9939-b964f3388e72`

</details>

<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between 2cc7523188364e193bd60807c8d408ef1796c09b and 5ba3fc30f55d1fd04fd3edeed23770dd84e302eb.

</details>

<details>
<summary>⛔ Files ignored due to path filters (1)</summary>

* `go.sum` is excluded by `!**/*.sum`

</details>

<details>
<summary>📒 Files selected for processing (32)</summary>

* `cmd/promote/cmd.go`
* `cmd/promote/dynatrace/dt_utils.go`
* `cmd/promote/dynatrace/dynatrace.go`
* `cmd/promote/dynatrace/utils_test.go`
* `cmd/promote/git/app_interface.go`
* `cmd/promote/git/app_interface_test.go`
* `cmd/promote/git/service_repo.go`
* `cmd/promote/git/service_repo_test.go`
* `cmd/promote/managedscripts/managed_scripts.go`
* `cmd/promote/managedscripts/managed_scripts_test.go`
* `cmd/promote/pathutil/pathutil.go`
* `cmd/promote/pathutil/pathutil_test.go`
* `cmd/promote/pko/pko.go`
* `cmd/promote/saas/saas.go`
* `cmd/promote/saas/saas_test.go`
* `cmd/promote/saas/utils.go`
* `cmd/promote/saas/utils_test.go`
* `cmd/promote/utils/app_interface_clone.go`
* `cmd/promote/utils/app_interface_clone_test.go`
* `cmd/promote/utils/git_repo.go`
* `cmd/promote/utils/service.go`
* `cmd/promote/utils/service_test.go`
* `cmd/promote/utils/services_registry.go`
* `cmd/promote/utils/services_registry_test.go`
* `cmd/promote/utils/test_tools.go`
* `cmd/promote/utils/utils_test.go`
* `docs/README.md`
* `docs/osdctl_promote.md`
* `docs/osdctl_promote_dynatrace.md`
* `docs/osdctl_promote_managedscripts.md`
* `docs/osdctl_promote_saas.md`
* `go.mod`

</details>

<details>
<summary>💤 Files with no reviewable changes (9)</summary>

* cmd/promote/pathutil/pathutil_test.go
* cmd/promote/saas/utils_test.go
* cmd/promote/pathutil/pathutil.go
* cmd/promote/git/service_repo.go
* cmd/promote/git/app_interface_test.go
* cmd/promote/git/service_repo_test.go
* cmd/promote/pko/pko.go
* cmd/promote/saas/utils.go
* cmd/promote/git/app_interface.go

</details>

<details>
<summary>✅ Files skipped from review due to trivial changes (3)</summary>

* docs/osdctl_promote_dynatrace.md
* cmd/promote/utils/service_test.go
* cmd/promote/utils/utils_test.go

</details>

<details>
<summary>🚧 Files skipped from review as they are similar to previous changes (6)</summary>

* cmd/promote/cmd.go
* docs/osdctl_promote.md
* go.mod
* cmd/promote/utils/services_registry.go
* cmd/promote/utils/services_registry_test.go
* cmd/promote/utils/test_tools.go

</details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->